Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bril2json -p -p and Rust/Clap updates #233

Merged
merged 7 commits into from
Nov 3, 2022
Merged

bril2json -p -p and Rust/Clap updates #233

merged 7 commits into from
Nov 3, 2022

Conversation

Pat-Lafon
Copy link
Contributor

  • All the rust tools now use Clap v4.0
  • Rust 1.64 Clippy lint updates
  • Link fix in docs
  • Minor bug in brilck where it didn't check if there were actually instructions in the function(func.instrs could be undefined) which triggered on test/check/badcall.bril for @nothing. Not sure why this wasn't caught before.
  • bril-rs adds support for Richer Source Positions #232
  • (rust) bril2json now takes multiple -p flags where just one -p is still compatible with python versions and -p -p additionally adds the end source positions.
  • rs2bril now supports end source positions via proc_macro2::Span(best effort) and also adds the --file/-f flag.
  • brilirs now has rough support for end source positions.

I've manually tested the additional support for richer source positions but like the rest of the error-handling support is not rigorously tested.

sampsyo
sampsyo approved these changes Oct 3, 2022
View reviewed changes
Copy link
Owner

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This all looks great!!

This is probably for a subsequent PR, but maybe we should just make the enhanced positions thing the default everywhere… that is, we could make the Python-based parser behave the same way, and alleviate the need for the -p -p mode in the Rust-based parser. Does that seem reasonable to you?

brilck.ts
err(`multiply defined label .${instr.label}`, instr.pos);
} else {
labels.add(instr.label);
if (func.instrs){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@Pat-Lafon Pat-Lafon mentioned this pull request Nov 2, 2022
Merged
@Pat-Lafon
Copy link
Contributor Author

This is probably for a subsequent PR, but maybe we should just make the enhanced positions thing the default everywhere… that is, we could make the Python-based parser behave the same way, and alleviate the need for the -p -p mode in the Rust-based parser. Does that seem reasonable to you?

Seems reasonable to me.

A separate question, maybe source positions should be spans by default(instead of starting positions by default) if all of the tools that support source positions provide starting and ending positions. This would mean making the pos_end field not optional(though providing positions as a whole is still optional).

@sampsyo
Copy link
Owner

sampsyo commented Nov 3, 2022

Yeah, seems absolutely reasonable to me.

@sampsyo sampsyo merged commit 98c3e39 into sampsyo:main Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sampsyo sampsyo sampsyo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Pat-Lafon @sampsyo